Skip to content

Conversation

@jkjk822
Copy link
Contributor

@jkjk822 jkjk822 commented Sep 22, 2025

Description

I'm not really sure why this HeadingNode code exists - it seems both semantically incorrect in all the cases I've observed and also causes bugs in other Lexical functions, as if they keep a reference to the HeadingNode that is replaced, subsequent usage of that HeadingNode will cause errors. See a common example of this in RangeSelection#insertNodes in the added test.

Also moved other regression tests to bottom of file. I can move this test case somewhere else if more appropriate.

Test plan

Test included - fails with invariant before this test.

Before

image

After

Test passes.

@vercel
Copy link

vercel bot commented Sep 22, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
lexical Error Error Sep 22, 2025 10:05pm
lexical-playground Ready Ready Preview Comment Sep 22, 2025 10:05pm

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 22, 2025
I'm not really sure why this is here - it seems both semantically incorrect in all the cases I've observed and also causes bugs in other Lexical functions, as if they keep a reference to the `HeadingNode` that is replaced, subsequent usage of that `HeadingNode` will cause errors. See a common example of this in `RangeSelection#insertNodes()` in the added test.
@etrepum
Copy link
Collaborator

etrepum commented Sep 22, 2025

I think this code exists so if you have a document that starts with a HeadingNode and move the cursor to the beginning of the document and press return then it will add an empty ParagraphNode before the HeadingNode instead of creating a second HeadingNode. I don't know why it does that (AFAICT Google Docs and Word do not behave like that), but I think that's the purpose. @zurfyx any historical knowledge here?

Maybe there's a bug where the selection anchor and focus isn't fixed up correctly during this operation, or insertNodes is holding on to a node reference longer than it should rather than going back to the selection. Those sorts of bugs could be fixed without changing the behavior here (and probably also fix other issues with other nodes that just haven't been reported yet).

@etrepum
Copy link
Collaborator

etrepum commented Sep 22, 2025

Note that there is an e2e test that is expecting the current behavior which fails https://github.com/facebook/lexical/actions/runs/17925833260/job/50971623711?pr=7846

 1 failed
    [chromium] › packages/lexical-playground/__tests__/e2e/TextEntry.spec.mjs:69:3 › TextEntry › Can type 'Hello' as a header and insert a paragraph before 

@jkjk822
Copy link
Contributor Author

jkjk822 commented Sep 22, 2025

FWIW, I believe firstBlock is the problematic reference for the specific case of RangeSelection#insertNodes. And this.insertParagraph() in "CASE 3" is where the reference is invalidated.

Maybe the reference should be updated there via another call to $getAncestor?

I think as a user I would personally prefer to match Docs and Word, but fixing the invariant bug is the main concern.

@etrepum
Copy link
Collaborator

etrepum commented Sep 22, 2025

I think that's right, it should be a let instead of a const and re-computed based on the selection after any mutation that might have removed it (can check isAttached). Might need to capture a reference to its previous sibling or something beforehand.

@jkjk822
Copy link
Contributor Author

jkjk822 commented Sep 22, 2025

Took a stab at it, but need some help. Trying to restore it based on the selected child does not seem to work, as the node we actually want to insert children into is the previous empty paragraph. I'm a bit confused why insertParagraph moves that out of our selection at all, but figured I'd ask for someone to take a look rather than also trying to change code there.

Maybe it does need to use previous sibling instead? But also seems weird that now lastInsertedBlock and insertedParagraph are the same block.

Things like `insertParagraph` can remove `firstBlock` from the editor state, so re-compute it in those cases.
@jkjk822
Copy link
Contributor Author

jkjk822 commented Sep 22, 2025

If I randomly add a getPreviousSibling() to the reassignment it passes all the tests and avoids lastInsertedBlock and insertedParagraph being the same block - but this doesn't seem to make sense semantically and probably breaks other things in subtle ways.

I'm going to let someone who understands the intent of the code better take a look. Happy to make changes if given suggestions around what correct behavior should look like here.

@jkjk822
Copy link
Contributor Author

jkjk822 commented Sep 30, 2025

@zurfyx @etrepum Any direction here? Should we 1) match Google Docs/MS Word by removing the header behavior (i.e. my first commit) 2) try harder to support this within current paradigm (i.e. fix my second commit) or 3) Give up and wait for caret refactor to hopefully fix the bug.

@etrepum
Copy link
Collaborator

etrepum commented Oct 2, 2025

So far as anyone remembers this decision was opinion-based and fairly arbitrary. Changing the behavior to match other popular text editors (google docs, word, etc.) would be fine, but of course the tests also have to change to match the updated behavior and the PR description should be updated to note the breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants